Automated Test: unified-storage-enhancements #347
Conversation
…#97529) * dont lazy init unified storage * Inits index when creating new resource server. Fixes trace propagation by passing span ctx. Update some logging. * Use finer grained cache locking when building indexes to speed things up. Locking the whole function was slowing things down. * formatting * linter fix * go mod * make update-workspace * fix workspaces check error * update dependency owner in mod file * wait 1 second before querying metrics * try with big timeout, see if fixes CI. Wont fail locally. * skips postgres integration test. Only fails in drone. Will fix later. * put delay back to 500 ms
📝 WalkthroughWalkthroughThis PR modernizes the codebase by migrating to slog-based logging, converting resource server initialization from lazy to eager pattern, improving context propagation through traced operations, and optimizing lock contention in index building to enable concurrent execution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/storage/unified/resource/search.go (1)
186-192:⚠️ Potential issue | 🔴 CriticalData race on
totalBatchesIndexedanderrinside concurrent goroutines.
totalBatchesIndexed++(line 189) is executed by multiple goroutines launched viaerrgroupwithout synchronization — this is a data race.Additionally,
_, _, err = s.build(...)(line 190) uses=which writes to the outererrvariable declared on line 181. Multiple goroutines writing to the same variable concurrently is also a data race. Use:=to scope it to the closure, or declare a local variable.🐛 Proposed fix
for _, info := range stats { group.Go(func() error { s.log.Debug("initializing search index", "namespace", info.Namespace, "group", info.Group, "resource", info.Resource) - totalBatchesIndexed++ - _, _, err = s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion) + _, _, err := s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion) return err }) }Then use
len(stats)or anatomic.Int32fortotalBatchesIndexedin thespan.AddEventcall below:- span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", totalBatchesIndexed))) + span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", len(stats))))
🤖 Fix all issues with AI agents
In `@pkg/storage/unified/sql/backend.go`:
- Around line 126-127: The span names are getting a double dot because
tracePrefix is "sql.resource." and the calls use tracePrefix+".GetResourceStats"
and tracePrefix+".Read"; update the concatenation in the b.tracer.Start calls
(e.g., where GetResourceStats and Read are started) to avoid the extra
dot—either remove the leading dot in the added literal (use "GetResourceStats" /
"Read") or trim the trailing dot from tracePrefix so span names become
"sql.resource.GetResourceStats" and "sql.resource.Read".
🧹 Nitpick comments (2)
pkg/storage/unified/search/bleve.go (1)
99-101: TODO left behind — track or address the RV check.The previous code in this block was replaced with a placeholder comment. If the last-RV optimization is important for avoiding unnecessary re-indexing of file-based indices, consider opening an issue to track it.
pkg/server/module_server_test.go (1)
35-38: Reasonable CI workaround — ensure the TODO is tracked.Skipping postgres in CI is pragmatic, but without a linked issue this TODO may be forgotten. Consider creating a tracking issue or adding a reference here.
,
Would you like me to open a GitHub issue to track fixing this test for postgres in Drone?
| ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats") | ||
| defer span.End() |
There was a problem hiding this comment.
Span name has a double dot: sql.resource..GetResourceStats.
tracePrefix is defined as "sql.resource." (line 27), so concatenating ".GetResourceStats" produces "sql.resource..GetResourceStats". The same issue exists on line 353 (tracePrefix+".Read"), while other methods in this file correctly omit the leading dot. Context propagation itself is correct and welcome.
🔧 Suggested fix
- ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats")
+ ctx, span := b.tracer.Start(ctx, tracePrefix+"GetResourceStats")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats") | |
| defer span.End() | |
| ctx, span := b.tracer.Start(ctx, tracePrefix+"GetResourceStats") | |
| defer span.End() |
🤖 Prompt for AI Agents
In `@pkg/storage/unified/sql/backend.go` around lines 126 - 127, The span names
are getting a double dot because tracePrefix is "sql.resource." and the calls
use tracePrefix+".GetResourceStats" and tracePrefix+".Read"; update the
concatenation in the b.tracer.Start calls (e.g., where GetResourceStats and Read
are started) to avoid the extra dot—either remove the leading dot in the added
literal (use "GetResourceStats" / "Read") or trim the trailing dot from
tracePrefix so span names become "sql.resource.GetResourceStats" and
"sql.resource.Read".
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Tests
Chores